Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

main, win: create test runner in windows #3

Closed
wants to merge 1 commit into from

Conversation

refack
Copy link
Contributor

@refack refack commented May 7, 2017

For this case, I could do it simpler than libuv (since the runner prints it's own errors)
Fixes: #1

Copy link
Owner

@indutny indutny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few nits, otherwise LGTM. Thank you!

mini-test.gyp Outdated
@@ -1,7 +1,7 @@
{
"targets": [ {
"target_name": "mini-test",
"type": "<!(gypkg type)",
"type": "static_library",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather keep it. Any point in dropping it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, local code leak.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to run "real" GYP to get Visual Studio project files 😞

src/spawn-win.c Outdated
static char executable_path[BUFSIZE];

int popen_complete(const char* cmd)
{
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style, please put { on the same line as popen_complete.

Let's make popen_complete static.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack

@@ -6,11 +6,20 @@
#include <string.h>
#include <errno.h>

#define BUFSIZE 4096
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather move BUFSIZE to some private file. This file is a public API.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So to src/spawn-win.c

#include <Windows.h>
#define ABORT ExitProcess(42)
#else
#define ABORT abort()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no abort on Windows? How do they live?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have but it makes allot of noise (Tries to find a debugger and creates a useless coredump)

src/spawn-win.c Outdated
PROCESS_INFORMATION ProcessInformation;
ZeroMemory(&ProcessInformation, sizeof(ProcessInformation));

// BOOL WINAPI CreateProcess(
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it sounds lame, but let's use /* */ comments here for consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NP


// Wait until child process exits.
DWORD wait_ret = WaitForSingleObject(ProcessInformation.hProcess, INFINITE);
CHECK_EQ(wait_ret, WAIT_OBJECT_0, "Process ended strange");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it happen on non-zero exit code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, only on strage cases

Return code/value Description
WAIT_ABANDONED 0x00000080L The specified object is a mutex object that was not released by the thread that owned the mutex object before the owning thread terminated. Ownership of the mutex object is granted to the calling thread and the mutex state is set to nonsignaled. If the mutex was protecting persistent state information, you should check it for consistency.
WAIT_OBJECT_0 0x00000000L The state of the specified object is signaled.
WAIT_TIMEOUT 0x00000102L The time-out interval elapsed, and the object's state is nonsignaled.
WAIT_FAILED 0xFFFFFFFF The function has failed. To get extended error information, call GetLastError.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. Thanks!

@refack
Copy link
Contributor Author

refack commented May 7, 2017

PTAL

Copy link
Owner

@indutny indutny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last nit, otherwise LGTM!

src/spawn-win.c Outdated
PROCESS_INFORMATION ProcessInformation;
ZeroMemory(&ProcessInformation, sizeof(ProcessInformation));

/*
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry for nit-picking again. But could it be indented?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried...


// Wait until child process exits.
DWORD wait_ret = WaitForSingleObject(ProcessInformation.hProcess, INFINITE);
CHECK_EQ(wait_ret, WAIT_OBJECT_0, "Process ended strange");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. Thanks!

@indutny
Copy link
Owner

indutny commented May 10, 2017

Landed with fixes in cb2b17e . Thank you!

Would you care to check that my fixes didn't break anything on Windows?

@indutny indutny closed this May 10, 2017
@refack
Copy link
Contributor Author

refack commented May 10, 2017

Works. Looked at the diff, you just bunched the declarations... seems semantically equal.

@refack refack deleted the win-comapt-#1 branch May 10, 2017 21:26
@indutny
Copy link
Owner

indutny commented May 10, 2017

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants